Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support explicit ttl #93

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

support explicit ttl #93

wants to merge 13 commits into from

Conversation

srinandan
Copy link
Contributor

The oauth plugin will support three new parameters:

  • cacheKeySize (to set the number of api keys that will be stored in cache)
  • cacheKeyTTL (to set the time-to-live for apikeys in cache; note: the entry will also be determined by the JWT expiry)
  • tokenKeyTTL (to set the time-to-live for oauth tokens in cache; note: the entry will also be determined by the JWT expiry)

The oauthv2 plugin will support a new parameter:

  • tokenKeyTTL (to set the time-to-live for oauth tokens in cache; note: the entry will also be determined by the JWT expiry)

The apikey plugin will support two new parameters:

  • cacheKeySize (to set the number of api keys that will be stored in cache)
  • cacheKeyTTL (to set the time-to-live for apikeys in cache; note: the entry will also be determined by the JWT expiry)

@WWitman
Copy link
Collaborator

WWitman commented Jan 6, 2019

This PR requires documentation updates.

@swilliams11
Copy link
Contributor

swilliams11 commented Jan 7, 2019

I reviewed the plugins and naming conventions look good. I agree with @WWitman that we need the docs and the plugin README files to updated.

@WWitman
Copy link
Collaborator

WWitman commented Jan 8, 2019

We can do a beta doc update for this feature and others that go into the next beta. But I'll need to have an idea of when the beta is scheduled, if possible, so I can plan for it.

@@ -35,6 +37,10 @@ module.exports.init = function(config, logger, stats) {
var keepApiKey = config.hasOwnProperty('keep-api-key') ? config['keep-api-key'] : false;
//cache api keys
cacheKey = config.hasOwnProperty("cacheKey") ? config.cacheKey : false;
//cache ttl
cacheKeyTTL = config.hasOwnProperty("cacheKeyTTL") ? config.cacheKeyTTL : 60000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't respect the default cacheKeyTTL value prev set on line 25 - avoid magic numbers. same for cacheSize

oauth/index.js Outdated
@@ -43,6 +46,10 @@ module.exports.init = function(config, logger, stats) {
var apiKeyHeaderName = config.hasOwnProperty('api-key-header') ? config['api-key-header'] : 'x-api-key';
var keepAuthHeader = config.hasOwnProperty('keep-authorization-header') ? config['keep-authorization-header'] : false;
cacheKey = config.hasOwnProperty('cacheKey') ? config.cacheKey : false;
//cache ttl
cacheKeyTTL = config.hasOwnProperty("cacheKeyTTL") ? config.cacheKeyTTL : 60000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't respect the default cacheKeyTTL value prev set on line 35 - avoid magic numbers. same for other cache vars

oauthv2/index.js Outdated
@@ -50,6 +51,8 @@ module.exports.init = function(config, logger, stats) {
}
//token cache settings
tokenCache = config.hasOwnProperty('tokenCache') ? config.tokenCache : false;
//token cache ttl
tokenCacheTTL = config.hasOwnProperty("tokenCacheTTL") ? config.cacheKeyTTL : 60000;
Copy link
Contributor

@relloller relloller Feb 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't respect the default tokenCacheTTL value prev set above avoid magic numbers. same for other cache vars

@srinandan
Copy link
Contributor Author

PTAL

@theganyo
Copy link
Member

It looks like this PR was abandoned and it has conflicts. (Also, it shouldn't be changing the version.) Was this in response to a specific feature request? Is it still needed? Or can this just be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants